-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(APIv2): RHINENG-13402 enable multidimensional sort in OpenAPI spec #2232
base: master
Are you sure you want to change the base?
fix(APIv2): RHINENG-13402 enable multidimensional sort in OpenAPI spec #2232
Conversation
cb09a04
to
c8cd0ef
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2232 +/- ##
=======================================
Coverage 98.83% 98.83%
=======================================
Files 232 232
Lines 5082 5082
=======================================
Hits 5023 5023
Misses 59 59 ☔ View full report in Codecov by Sentry. |
c30ebcb
to
852b0df
Compare
@@ -158,7 +158,7 @@ def sort_params(model = nil) | |||
end | |||
|
|||
def sort_params_v2(model = nil, except: []) | |||
parameter name: :sort_by, in: :query, required: false, | |||
parameter name: :sort_by, in: :query, required: false, style: :form, explode: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the style: :form
needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following rswag documentation: https://github.com/rswag/rswag/wiki/FAQ#for-openapi-3
https://swagger.io/docs/specification/v3_0/serialization/#query-parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, still don't understand why is this needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
form – (default) ampersand-separated values, also known as form-style query expansion. Corresponds to the {?param_name} URI template.
I believe this is the option we want to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's default, why do we need this now? The format checks out, we only need the explode
as far as I understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And why just here be explicit? If it's a default value, it should be consistent across all parameters in the whole specification. So either change it everywhere to style: form
or please delete this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want this to be applied on those parameters of type: :array
. Where else do you see this missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is default (which is), it means it is applied to everything, not just the type: :array
so I am asking again, why is this case so special that you want to have an explicit setting that is already implicitly set on it? Why and how is this one single case different from all the other parameters that may or may not have a type: :array
which is not connected to the formatting that you are trying to set now explicitly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The referenced ticket answers your question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it doesn't, you have 2 independent attributes that you are for some reason try to bind into a single context. One of them is default, so its setting is irrelevant and to maintain consistency across the OpenAPI spec, it makes no sense to explicitly specify in a single location.
852b0df
to
3160480
Compare
/retest |
3160480
to
dc7f3e5
Compare
dc7f3e5
to
8a9fdda
Compare
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist